Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing schedules in add_systems doc (#11814) #11815

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Purfakt
Copy link

@Purfakt Purfakt commented Feb 10, 2024

Objective

Fixes (#11814)

Solution

I added Update where it was missing in the rustdoc.

Uncovered problem

The previous doc was mocking the app as @hymm said:

app doesn't actually exist in bevy_ecs. It's just mocked in the doc comments as a schedule.

Proposed solutions

  • @pablo-lua suggested to just rename app in schedule and let the example be correct without the changes of the PR as it is, in fact, a schedule

  • @hymm suggested to move the add_systems impl on App onto World and just use that on the impl on App

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@Purfakt
Copy link
Author

Purfakt commented Feb 10, 2024

I'm not sure I understand why, when I try to run the rustdoc, I get:

error[E0425]: cannot find value `Update` in this scope
  --> crates/bevy_ecs/src/system/combinator.rs:57:5
   |
40 |     Update,
   |     ^^^^^^ not found in this scope

error[E0061]: this method takes 1 argument but 2 arguments were supplied
   --> crates/bevy_ecs/src/system/combinator.rs:56:5
    |
39  |    app.add_systems(
    |        ^^^^^^^^^^^
40  |        Update,
    |  ____________-
41  | |/     my_system.run_if(Xor::new(
42  | ||         IntoSystem::into_system(resource_equals(A(1))),
43  | ||         IntoSystem::into_system(resource_equals(B(1))),
44  | ||         // The name of the combined system.
45  | ||         std::borrow::Cow::Borrowed("a ^ b"),
46  | || )));
    | ||  -
    | ||__|
    |  |__help: remove the extra argument
    |     unexpected argument of type `NodeConfigs<Box<(dyn bevy_ecs::system::System<In = (), Out = ()> + 'static)>>`
    |
note: method defined here
   --> /home/purfakt/dev/perso/bevy/crates/bevy_ecs/src/schedule/schedule.rs:249:12
    |
249 |     pub fn add_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) -> &mut Self {
    |            ^^^^^^^^^^^

error: aborting due to 2 previous errors

@hymm
Copy link
Contributor

hymm commented Feb 10, 2024

This is because app doesn't actually exist in bevy_ecs. It's just mocked in the doc comments as a schedule. Not sure how to fix this.

@pablo-lua
Copy link
Contributor

pablo-lua commented Feb 10, 2024

I think this can't be solved unless we import bevy_app into bevy_ecs (that would probably defeat the purpose of bevy_ecs being independent)
An possible option is changing the var name from app to something else

Here we have the definition of the var:

/// # let mut app = Schedule::default();

We would first remove the # to clear that we are not talking about the App here and rename app to schedule

/// let mut schedule = Schedule::default();

That would clear up the problem, I think

@hymm
Copy link
Contributor

hymm commented Feb 10, 2024

We could move the add_systems impl on App onto world and just use that on the impl on App. More code duplication, but might be worth it.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Feb 10, 2024
@Purfakt
Copy link
Author

Purfakt commented Feb 10, 2024

Oh I see. I'm not sure if I'm capable or entitled to make this change.

I didn't expect my first PR to be tagged controversial. Especially, when it was just a documentation update!

@pablo-lua
Copy link
Contributor

Oh I see. I'm not sure if I'm capable or entitled to make this change.

I didn't expect my first PR to be tagged controversial. Especially, when it was just a documentation update!

Well, this means that the issue is rather good, as this shows that there is a misunderstood going about what is the app that the documentation is talking about, a third opinion might be good here in regarding of what to do for sure.

@hymm
Copy link
Contributor

hymm commented Feb 11, 2024

I wouldn't mind having this pr change the examples in bevy_ecs to use schedule, so at least they're correct. We can make an issue for a follow up to discus adding a add_systems method to world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants